remove unordered_map for attr handler #805
Conversation
|
could you please provide a PR description? i know there were discussions about the performance implications of this change, it'd be good to have those documented as part of this PR |
| const auto ucode_map = ucode_attr_handlers[ucode_handler_index]; | ||
| assert(ucode_map->contains(attr) && "Attribute not found in the current ucode handler"); | ||
| return std::any_cast<T>(ucode_map->at(attr)); | ||
| // assert(ucode_map->contains(attr) && "Attribute not found in the current ucode handler"); |
There was a problem hiding this comment.
why is this commented out?
There was a problem hiding this comment.
ucode_map are no longer a map so we no longer need to do the assert.
what do you mean by document it ? because it's just change an unordored_map<int, int> where the key are an enum with limited range and static so I just change it to use instead an array so not so much change. |
the what aspect of this PR is clear, but the why is not documented. i remember conversations about the why relating to performance. it'd be good to document performance gains from this change and weigh them against the original reasoning behind the architectural decision to use a map (it looks like these maps were added in #557 and updated in #567 - @KiritoDv do you have thoughts on this one?) |
|
originally it use an unordered_map because @KiritoDv want just to make it work, without really caring about performance, by changing to just an array to allow to access more directly to the ressource (so avoid all process of an hash table). I will ping you in the private channel where we discuss about that. |
change ucode_attr_handlers to an array of array instead of map to no longer compute hash.